fix(cli/doctor-pi): skip unrelated local packages when probing embedding runtime - #414
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Both review findings addressed in 296e340:
Also added Test run: 20 tests, 17 pass, 3 pre-existing failures (DB schema / legacy config migration, reproduced on clean master). |
There was a problem hiding this comment.
Thanks for isolating this — the premise holds (on master piPluginDirCandidates probes every local-path entry and aborts on the first broken one), the package-name filter is the right direction, and your regression is red on master. Three asks before merge:
-
Pick the best result across all candidates, native OK > WASM OK > broken. The loop still stops on an earlier
wasm-fallback, so an earlier local tree with a broken native binding but working WASM makes a later healthy native managed install report as degraded (greptile's P1 — I reproduced it with a variant of your test). Please add that exact regression: broken-native + working-WASM candidate first, healthy native candidate later, assert the nativePASS. -
When every candidate fails, name every candidate. Only
firstBrokensurvives into the warning today; the all-broken message should list each path with its native/WASM reason so the user can tell which tree is stale. -
Treat a local dev path as a second loaded identity. Pi keys
npm:@cortexkit/pi-magic-contextand a local directory whosepackage.jsoncarries that name as different identities and loads both; the duplicate-load conflict check still only recognizes the npm spelling, so the doctor output should not imply the managed install is the sole runtime that will load. Please make the conflict diagnostic identity-aware and cover the local+npm case.
One formatter violation at :801-803 — run the repo's pinned biome (packages/cli/node_modules/.bin/biome check). On this Mac the CLI suite is 385/0 on your head; I could not verify the three Windows-only failures you mention, so if they are real please paste them.
|
Rechecked
The current head also fails the repository lint gate on formatting in both changed files. Typecheck passes, and the focused doctor test passes 20/20 (86 expectations). The branch is hundreds of commits behind current master and GitHub currently shows reviewer/security checks rather than repo CI. Recommendation: keep open, rebase, fix those gaps, and rerun CLI lint/typecheck/tests before merge. |
…ing runtime piPluginDirCandidates treated every non-npm: entry in Pi packages[] as a candidate plugin tree, so local dev-path extensions (any package.json, regardless of name) were probed for the embedding runtime. The first broken candidate made doctor report 'native runtime and WASM fallback both unavailable' and stop, even when the real magic-context install was healthy. Now only directories whose package.json names @cortexkit/pi-magic-context qualify as candidates, and broken candidates no longer abort the scan — a stale local dev tree cannot mask a healthy managed install. Repro: register any local-path Pi extension (D:\repo\my-extension) in settings.json packages[], run 'doctor --harness pi' — doctor blamed the extension's package.json for missing onnxruntime-web deps instead of reporting the actual plugin install.
…nstall Address review findings (Greptile P1, cubic-dev-ai P2): the loop still stopped at the first candidate with a working WASM fallback, reporting a degraded runtime even when a later managed install had the native binding. Record the best degraded candidate and keep probing; only report the fallback WARN when no candidate is fully OK. Tests: add regression coverage for (1) unrelated local packages never probed (unverified, not a broken-runtime WARN, when only unrelated packages are registered), and (2) a WASM-only dev tree not masking a later native-capable install.
296e340 to
8ba8512
Compare
|
Addressed the two remaining findings in 8ba8512 and rebased onto master 7680bd8.
CLI typecheck and diff checks pass. Repository Biome passes all 91 CLI files in an LF-normalized temporary copy; the Windows checkout itself produces CRLF-only lint diagnostics in 89 untouched files. Both PR files were formatted with the repository-installed Biome. Here are the requested Windows doctor failures, each reproduced independently on clean master 7680bd8: Full doctor: 22 pass / 3 fail. Full CLI runner: 345 pass / 5 skip / 31 fail versus master 339 pass / 5 skip / 31 fail, with identical failure-name sets. I also ran all four isolated test files that the runner skips after a failure; their results match master. Aggregate coverage is 365 pass / 11 skip / 35 fail versus master 359 pass / 11 skip / 35 fail. The PR description contains the per-file breakdown and dependency setup caveat. I am not claiming green Windows repository gates or local macOS/Linux validation. Please re-review the two fixes; cross-platform CI/review is still needed. |
There was a problem hiding this comment.
Re-reviewed 8ba8512 by execution on macOS. All three asks are closed:
- Candidate precedence: the loop now
continues past a WASM fallback and past broken trees, keeps the first fallback only as the degraded result, and a later healthy native install wins. The new regression "prefers a later native-capable install over an earlier WASM fallback" is the exact case from the first review. - All-broken reporting: every failed candidate is retained with its path and its native/WASM reason, one warning per candidate ("reports every broken candidate with its native and WASM reasons").
- Duplicate-load identity: a local directory whose package.json names
@cortexkit/pi-magic-context(string or{ source }form, absolute or agent-dir-relative) now counts as a loaded Magic Context identity in both candidate discovery and the conflict check, so npm + local registrations report the conflict while a healthy embedding PASS is still reported.
Gates on the head: CLI typecheck clean, pinned Biome clean on both files, doctor-pi 25/0, full CLI suite 379 pass / 2 skip / 0 fail (the skips are Windows-only). The three Windows failures you listed reproduce on clean master per your run and are unrelated to this change; they are ours to chase separately. Merging. Thanks for the careful follow-through across three rounds.
Problem
Pi doctor probes unrelated local-path extensions for embedding dependencies, and an earlier broken or WASM-only tree can mask a later healthy native installation. Its duplicate-load diagnostic also misses npm + local Magic Context registrations, and when all candidates are broken it reports only the first failure.
Changes
@cortexkit/pi-magic-contextname in package.json; share local-path resolution between embedding candidate discovery and duplicate-load/other-extension diagnostics, including object-form{ source }entries.Rebased onto master
7680bd8fd06f015a905b7e00f37fa107754f84ba.Validation
Windows, Bun 1.3.14:
bun run typecheckandgit diff --check: pass.check srcagainst a temporary LF-normalized copy: 91 files pass. Ordinary Windows checkout lint reports CRLF formatting on 89 untouched files; no unrelated files were reformatted in this PR.bun run test: 345 pass / 5 skip / 31 fail, compared with clean master 339 pass / 5 skip / 31 fail; failure-name sets are identical.Reproduced doctor failures on Windows
leaves an older supported shared DB schema unchanged: cleanuprmSyncfails withEBUSY: resource busy or lockedon the temporary DB directory.migrates legacy Pi user config before --force writes a default: expectedprotected_tokensto be13; receivedundefined.does not write a default when legacy user configs conflict: expected the target config to be absent (false); receivedtrue.The complete repository gates are not green on this Windows host. These failures match the tested master baseline; their root causes have not been exhaustively diagnosed. macOS/Linux were not run locally in this follow-up.
Dependency setup used the frozen lockfile with install scripts disabled after the configured mirror failed to provide an unrelated OpenCode Windows binary. The lockfile is unchanged.
Greptile Summary
The PR fixes Pi doctor’s embedding-runtime candidate selection and completes the prior fallback-scanning fix.
@cortexkit/pi-magic-contextpackage identity.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Pi packages and managed roots] --> B[Build plugin candidates] B --> C{Magic Context package?} C -->|No| D[Skip candidate] C -->|Yes| E[Probe embedding runtime] E -->|Native OK| F[Report PASS and stop] E -->|WASM fallback| G[Remember fallback and continue] E -->|Broken| H[Remember warning and continue] E -->|Unknown| I[Remember reason and continue] G --> J{More candidates?} H --> J I --> J J -->|Yes| E J -->|No| K[Report best deferred result]Reviews (3): Last reviewed commit: "fix(cli/doctor-pi): report broken candid..." | Re-trigger Greptile